-
Notifications
You must be signed in to change notification settings - Fork 40
Committee aggregation #282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
use committee aggregated signature proofs
also rename gossip_signatures to gossip_committee_signatures
jihoonsong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Please excuse leaving some comments while it's still in draft. Just wanted to help iterate faster :)
| # Configure the genesis state. | ||
| genesis_config = Config( | ||
| genesis_time=genesis_time, | ||
| attestation_subnet_count=AGGREGATION_COMMITTEE_COUNT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit that I was the one who advocated for attestation committee, but based on the fact that validators only push their attestations to aggregators in their subnet without subscribing to it, I now think aggregation committee gives us slightly better description.
I don't mind whichever we choose—either attestation committee or aggregation committee—but I do think we need to stick to one thing consistently in the Lean spec and pq-devnet-3.md in the pm repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong preference too, however I think the rationale for topic names in beacon chain spec is based on the type of messages that are being propagated to this topic. For consistency we should probably stick to the same logic and keep using attestation subnets and attestation committees
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great! In the same vein, what do you think about this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, applied your suggestion
# Conflicts: # src/lean_spec/subspecs/forkchoice/store.py # src/lean_spec/subspecs/networking/__init__.py
docs/client/validator.md
Outdated
|
|
||
| When aggregation is added, aggregators will collect attestations and combine them. | ||
| Aggregated attestations will be broadcast separately. | ||
| Devnet-2 introduces signatures aggregation. Aggregators will collect attestations and combine them. Aggregated attestations will be broadcast separately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Devnet-2 introduces signatures aggregation. Aggregators will collect attestations and combine them. Aggregated attestations will be broadcast separately. | |
| Devnet-3 introduces signatures aggregation. Aggregators will collect attestations and combine them. Aggregated attestations will be broadcast separately. |
…nt in configuration
Aggregate during interval 2 if more threshold signatures were received
docs/client/validator.md
Outdated
|
|
||
| In the devnet-3 design, however, there is one global subnet for signed | ||
| attestations propagation, in addition to publishing into per committee subnets. | ||
| This is due to 3SF-mini consensus design, that requires 2/3+ of all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this global bit is not required, once the aggregtors publish signed attestations in the 2nd interval, they can be imported by all validators in the 3rd interval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, without global topic for attestations, we might not be able to receive proofs in time to update safe target during interval 2:
- Interval 0: block propagation
- Interval 1: votes propagation
- Interval 2: signatures aggregation (up to one second for 1000 validators in subnet with 1000sigs/second expected sigs aggregation rate) + proof distribution => No time for updating safe target => in next slot validator votes for old target
| for data, validator_ids in data_to_validator_ids.items() | ||
| ] | ||
|
|
||
| class SignedAggregatedAttestation(Container): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anshalshukla / @GrapeBaBa do we already have this type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also better to use message, signature terminlogy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need aggregated bit vector here as well,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need aggregated bit vector here as well,
AggregatedSignatureProof contains AggregationBits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anshalshukla / @GrapeBaBa do we already have this type?
no
…n propagation in committee subnets
# Conflicts: # src/lean_spec/subspecs/networking/gossipsub/__init__.py # src/lean_spec/subspecs/node/node.py # tests/lean_spec/helpers/__init__.py # tests/lean_spec/subspecs/chain/test_service.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diffs are very large and spread out now. I suggest we rebase and merge this asap and do incremental reviews after.
# Conflicts: # src/lean_spec/subspecs/containers/state/state.py # src/lean_spec/subspecs/forkchoice/store.py # src/lean_spec/subspecs/networking/gossipsub/__init__.py # src/lean_spec/subspecs/validator/service.py
Co-authored-by: Unnawut Leepaisalsuwanna <921194+unnawut@users.noreply.github.com>
…mittee-aggregation
🗒️ Description
Introduces aggregator role and subnet aggregation.
* If aggregators collected 90% of signatures from their subnet by the beginning of slot 2, they produce aggregated attestation and propagate it into aggregation topiclatest_new_attestationsandlatest_known_attestationswere replaced withlatest_new_aggregated_payloadsandlatest_known_aggregated_payloads🔗 Related Issues or PRs
leanEthereum/pm#56
leanEthereum/pm#58
✅ Checklist
toxchecks to avoid unnecessary CI fails:uvx tox